Repository navigation
fix(warehouse-sources): stamp Delta commit times in the snapshot pin test - #93155
Conversation
|
😎 Merged successfully - details. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 CI report
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6287f6b149
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 2 @ 32945a2)Re-review after the helper docstring became a comment block. No actionable findings; the router judged the change low-danger with high confidence and delegated nothing. Key findings
Convergence
Reviewer summaries
Previous rounds (1)
Automated by QA Swarm — not a human review |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Test-only change (no production code) that fixes a flaky assertion by stamping Delta commit-file mtimes instead of relying on coarse filesystem clocks; contained and reversible, well outside risky territory, so no independent assurance is required despite cross-team authorship.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 23L/1F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (23L, 1F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 32945a2 · reviewed head 32945a2 |
…test test_resolve_pins_to_last_completed_snapshot_while_parent_is_syncing pinned at the v0 commit timestamp from DeltaTable.history() and expected v0 back. delta-rs time travel compares against the _delta_log commit files' mtimes instead, and on Linux those come from the kernel's coarse clock, so on a fast runner both commits can look older than v0's own timestamp and the pin lands on v1. The test failed 3/3 attempts in two merge-queue runs and blocked unrelated PRs. Stamp the two commit files with explicit, well-separated times and pin between them, which is also the production shape: finished_at sits strictly between the completed job's last commit and the next run's first one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…mment Python test modules do not carry doc comments in this repo; the delta-rs mtime rationale stays as an ordinary comment on the helper. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Test-only fix for a flaky mtime-dependent assertion, no production code touched, already carries a bot approval and a human approval with no unresolved concerns — not risky territory so cross-team authorship doesn't matter.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 29L/1F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (29L, 1F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ aad15f1 · reviewed head aad15f1 |
aad15f1 to
87a83f3
Compare
…ularity flakes A warehouse-sources test that pinned a Delta snapshot at the previous write's history() timestamp failed 3/3 attempts on the queue runner, passed on master at the same base SHA, and Trunk's bisection blamed an unrelated PR (#92922, fixed in #93155). The skills covering that path had three gaps: no rule against deriving a cutoff from an earlier write's recorded time, no recognition that an environment-dependent race fails every rerun yet passes elsewhere, and no account of Trunk's bisection branches or of the warehouse-sources exclusion from the quarantine gate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…mit gap Adopts the naming and timing from #93192: the helper says it sets commit log mtimes and takes the whole timeline in one call, and the two commits sit 2 minutes apart with the pin 1 minute after the first. Generated-By: PostHog Desktop Task-Id: 4c55bc40-1d25-42fe-9036-908e21493bd1
A new stamphog review started for this PR — the fresh verdict replaces this approval.
Generated-By: PostHog Desktop Task-Id: 7ee59145-d7a0-41f6-8de6-ab58a8cc10a0
There was a problem hiding this comment.
Approved.
Test-only fix (no production code touched) that stamps explicit Delta commit-log mtimes instead of relying on unreliable filesystem clock behavior; diff matches the description exactly, and a human reviewer already approved. Not risky territory, so cross-team authorship is just a routing note, not a blocker.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 27L/1F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (27L, 1F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 38660af · reviewed head 38660af |
…test (#93155) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…test (#93155) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…test (#93155) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
… snapshot pin test The merge kept this branch's busy-wait next to master's explicit mtime stamping (#93155), which removed the timestamp the wait referenced. Master's fix supersedes the wait.
Problem
Anyone whose PR shares a Trunk merge-queue batch with a warehouse-sources shard can get thrown out of the queue by
test_resolve_pins_to_last_completed_snapshot_while_parent_is_syncingfailing withassert 1 == 0. It failed 3/3 attempts in both the batch run and the bisection run for #92922 (a cohorts-only PR), while master's own CI on the identical base commit (7e12bc8) passed.The test writes a Delta table (v0), takes v0's commit timestamp from
DeltaTable.history(), immediately overwrites (v1), then pins at that timestamp and expects v0. But delta-rs time travel does not compare againsthistory()timestamps:get_version_timestamp/version_timestampin delta-rs 1.6.1 return the_delta_logcommit files'last_modified. On Linux, file mtimes come from the kernel's coarse clock and can lag the in-process clock by a scheduler tick, so on a fast runner both commit files can look older than v0's own timestamp. The binary search then lands on v1. Reruns don't help because the outcome tracks the runner's speed, not luck, and it never reproduces on macOS, where mtimes are fine-grained.Changes
commitInfo.timestampto v0's plus one millisecond. Delta-rs never reads that field for time travel, so the rewrite only worked because writing the file refreshed its mtime, and a probe confirmed it: with the file's mtime set back to the lagging value the pin still lands on v1. It also left the test without a comment saying why it does what it does.finished_atsits strictly between the completed job's last commit and the next run's first one. The commits sit 2 minutes apart and the pin 1 minute after the first — the helper name and those offsets come from fix(warehouse-sources): pin the parent snapshot test to a stable clock #93192, which fixed the same test independently and is closed in favor of this PR._set_delta_commit_log_mtimeshelper carries a comment on why delta-rs needs this, so the next person doesn't rediscover it. Thejsonimport fix(posthog-ai): convert remaining schema references to generated column tables #84280 added goes away with the rewrite it served.No production code changes.
How did you test this code?
APIBaseTest-based classes in the file error locally only because no Postgres is running, which is unrelated to this change.history()timestamp by 1ms,load_as_version(v0_timestamp)loads version 1; with the explicit stamps used here, it loads version 0.Note for a follow-up, not changed here: the
resolve_parent_table_refdocstring describes the rollback as comparingfinished_atagainst "Delta commit timestamps stamped by worker clocks". Against S3 it actually compares against S3'sLastModified, which is S3's clock at second granularity.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Fable 5.1). Diagnosis started from the failed merge-queue job link: read the shard logs, compared the queue branch's base SHA against master's check runs, read the pinned delta-rs source for
load_with_datetime, and reproduced the mtime-lag mechanism in a scratch script before touching the test. No repo skills were invoked. Skill updates capturing the lesson (writing-tests, fixing-flaky-tests, triaging-merge-queue-failures) are in a separate PR.🤖 Generated with Claude Code