Skip to content

Keep stack order stable on refresh - #15926

Open
mtsgrd wants to merge 2 commits into
masterfrom
stack-order-stability
Open

mtsgrd wants to merge 2 commits into
masterfrom
stack-order-stability

Conversation

@mtsgrd

@mtsgrd mtsgrd commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Stacks could change order on refresh for two unrelated reasons. One commit each, and each has a test that failed before its fix.

1. Moving a graph connection put its stack at the front

  • Stack order is the order of the workspace's outgoing connections; petgraph visits the newest first.
  • move_incoming_edges re-added the one connection it moved, so that stack jumped to the front. It runs after stacks are ordered.
  • It now rebuilds all of the source's outgoing connections in order. That covers all three callers: the two worktree paths, and the entrypoint path that needs no worktree.
Saved:            A, B   (worktree on B)
Refresh, before:  B, A
Refresh, after:   A, B

2. but commit saved the stack order after releasing its lock

  • The TOML store writes on drop. but commit re-binds its lock guard after meta, so the lock goes first and the TOML lands a moment later. The other seven transaction commands drop in the safe order.
  • The app's watcher waits on that lock and re-reads the moment it is free: new refs, old order. The TOML write then triggers a second re-read.
  • but commit -b <new> asks for position 0, so the new stack showed up last and then jumped to the front.
  • RefMetadata::flush (no-op by default, the TOML store overrides it) now runs inside the transaction, once the refs have moved. Drop order no longer matters.
  • A failed flush is logged, not returned: the refs already moved, and drop would only have logged too.
  • Swapping two lets in commit.rs would fix today's case. Flush also covers with_transaction, which always releases its own lock before the caller's handle drops.
Requested:  A, N, B
Before:     A, B, N, then A, N, B on drop
After:      A, N, B

Not covered

  • The second fix fits the jump I see in Lite while agents commit from the CLI, but I haven't measured it in a running app.
  • A read that doesn't wait for the lock can still land between the refs moving and the flush.

Verified

cargo test -p but-graph worktree_ref_as_later_stack_top_preserves_stack_order
cargo test -p but-transaction creating_stack_persists_order_before_metadata_is_dropped
cargo test -p but-meta flushed_metadata_remains_writable_until_drop

# The other workspace response format.
cargo test -p but-transaction --features but-api/graph-workspace

@github-actions github-actions Bot added the rust Pull requests that update Rust code label Sep 11, 2026
@mtsgrd
mtsgrd force-pushed the stack-order-stability branch from 01c5ac4 to b03cf20 Compare September 11, 2026 13:21
@mtsgrd

mtsgrd commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@Caleb-T-Owens I think this is what I've brought up in the past, would be great if you could have a look?

@mtsgrd
mtsgrd force-pushed the stack-order-stability branch from b03cf20 to d8a238b Compare September 11, 2026 13:37
@mtsgrd
mtsgrd marked this pull request as ready for review September 14, 2026 21:58
Copilot AI lite review requested due to automatic review settings September 14, 2026 21:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical failure paths can leave persistence layers inconsistent and Git ref changes unapplied to rollback.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR preserves workspace stack ordering and flushes transaction metadata before completion.

Changes:

  • Preserves graph edge traversal order during worktree projection.
  • Adds explicit metadata flushing for TOML-backed stores.
  • Adds regression tests for ordering, persistence, dry runs, and rollback.

The review found critical failure paths that can leave metadata layers inconsistent or apply Git ref changes despite a returned error.

File summaries
File Description
crates/but-transaction/src/tests.rs Tests persisted ordering and transaction behavior.
crates/but-transaction/src/lib.rs Flushes metadata before returning transaction state.
crates/but-meta/tests/meta/ref_metadata_legacy.rs Tests flushing and continued writes.
crates/but-meta/src/legacy/mod.rs Implements metadata-store flushing.
crates/but-graph/tests/graph/init/with_workspace.rs Tests cached and fresh graph ordering.
crates/but-graph/tests/fixtures/scenarios.sh Adds a linked-worktree ordering fixture.
crates/but-graph/src/init/post.rs Preserves edge order during retargeting.
crates/but-core/src/lib.rs Defines the metadata flush contract.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/but-meta/src/legacy/mod.rs
Comment thread crates/but-transaction/src/lib.rs Outdated
- Preserve stack order when separating worktree branches.
- Save workspace metadata before a transaction returns.
The refs are already updated when metadata is flushed, so returning an error
reported a completed change as failed and skipped its undo entry.

- Log the failure, as the TOML store does on drop, and return the workspace state.
- Test with a store whose save always fails: the reword lands and stays undoable.
Copilot AI review requested due to automatic review settings September 14, 2026 23:27
@mtsgrd
mtsgrd force-pushed the stack-order-stability branch from d8a238b to a2faf15 Compare September 14, 2026 23:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Metadata may be flushed before fallible workspace-state construction, allowing failed transactions to partially commit.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 9/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/but-transaction/src/lib.rs
@mtsgrd mtsgrd changed the title Preserve stack order across worktree projection and transaction completion Keep stack order stable on refresh Sep 21, 2026

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

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants