Skip to content

Drop deprecated lanes.size columns (PP-4506) - #3563

Draft
jonathangreen wants to merge 1 commit into
chore/has-root-lanes-scalar-countfrom
chore/drop-lane-size-columns
Draft

Drop deprecated lanes.size columns (PP-4506)#3563
jonathangreen wants to merge 1 commit into
chore/has-root-lanes-scalar-countfrom
chore/drop-lane-size-columns

Conversation

@jonathangreen

@jonathangreen jonathangreen commented Jul 15, 2026

Copy link
Copy Markdown
Member

Description

Release-2 (the drop) of the online-migration sequence for removing the deprecated lane-size cache columns. Stacked on the release-1 PR #3619, which stops all reads of size / size_by_entrypoint (they were already made write-safe via server_default in #3481). This PR removes the columns from the Lane model and drops them from the schema.

Do not merge until a release containing #3619 has shipped. Until then the still-running previous release reads lanes.size and dropping the column would break it — so this is left in draft. Note that the CI Backwards compatibility test on this PR is expected to fail until then: that job runs the latest published release's suite, which still reads the columns; it will pass once a release containing #3619 becomes the baseline.

Motivation and Context

JIRA: PP-4506

Completes the two-release removal. The read-side fix in #3619 was validated against this exact drop locally with the backwards-compatibility check's new local mode (#3618): the release-1 branch's pytest -m db suite passes against this dropped schema.

How Has This Been Tested?

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen added the DB migration This PR contains a DB migration label Jul 15, 2026
@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jonathangreen's task in 5m 27s —— View job


Code Review

  • Gather context (PR diff, CLAUDE.md online-migration conventions)
  • Review migration file (de6ae4bbf4a5)
  • Review Lane model changes
  • Verify no remaining references to the dropped columns
  • Verify release-1 (base branch) truly stops selecting the columns
  • Post review

Summary

This is a correct and tidy release-2 drop. I verified the pieces that actually matter for an online drop: the revision chain is linear and single-headed (de6ae4bbf4a5dbeb42224c1ba6c85605404c), the downgrade() faithfully restores the post-release-1 state (size NOT NULL with server_default 0 so existing rows backfill, size_by_entrypoint nullable json) rather than the pre-release-1 state — which is the right target, since the subsequent dbeb42224c1b.downgrade() is what strips the server default. No index, constraint, view, trigger, or raw SQL references either column, and the model has no leftover uses of the now-removed text / JSON / deferred imports.

Most importantly, I checked the N-1 side rather than taking the backwards-compat run on faith: on the base branch (c7dbfe9), the only count() calls over Lane are custom_lists.py:474 and library.py:397, both already narrowed to query(Lane.id), and affected_by_customlist() is only ever iterated in production code (custom_lists.py:395) — never .count()ed, .subquery()d, or unioned. So no release-1 query re-selects the deferred columns, and Lane carries no Python-side default for size, so its inserts omit the column. The drop is safe once release-1 ships. Skipping a per-migration test matches the existing convention for pure column drops (4c2d754c04e9, 6212e80c0fab have none), and the draft state + DB migration label + JIRA key in the title all follow CLAUDE.md.

No blocking or minor findings.
Branch: chore/drop-lane-size-columns

@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This is the second step of a two-release online migration sequence that removes the deprecated lanes.size and lanes.size_by_entrypoint cache columns. Release 1 (PR #3619) had already made all reads/writes safe by marking the columns as deferred and moving the size default into the database; this PR completes the cleanup by dropping them from both the ORM model and the schema.

  • Alembic migration (de6ae4bbf4a5): upgrade() drops size_by_entrypoint then size from the lanes table; downgrade() faithfully restores both columns in their release-1 state (size as NOT NULL with server_default=0, size_by_entrypoint as nullable JSON).
  • ORM model (lane.py): Removes the two deferred column mappings and the now-unused imports (text, JSON, deferred).

Confidence Score: 5/5

Safe to merge once a release containing PR #3619 has shipped; the migration chain is correct and no stale references to the dropped columns remain in the codebase.

The drop migration chains correctly from the prior add-server-default revision, the downgrade faithfully restores both columns in their pre-drop state, and a codebase-wide search confirms zero remaining references to lanes.size or lanes.size_by_entrypoint in src/ or tests/.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
alembic/versions/20260715_de6ae4bbf4a5_drop_deprecated_lanes_size_columns.py New migration that drops lanes.size and lanes.size_by_entrypoint; down_revision correctly chains to the add-server-default migration, and downgrade() accurately reconstructs both columns in their pre-drop state.
src/palace/manager/sqlalchemy/model/lane.py Removes the two deprecated deferred column definitions and their associated imports (text, JSON, deferred); no stale references to size or size_by_entrypoint remain anywhere in src/ or tests/.

Sequence Diagram

sequenceDiagram
    participant R1 as Release 1 (PR #3619)
    participant DB as Database
    participant R2 as Release 2 (this PR)

    Note over R1,DB: Migration dbeb42224c1b
    R1->>DB: "ALTER COLUMN lanes.size ADD server_default=0"
    Note over R1: Mark size & size_by_entrypoint as deferred

    Note over DB,R2: Migration de6ae4bbf4a5 (this PR)
    R2->>DB: DROP COLUMN lanes.size_by_entrypoint
    R2->>DB: DROP COLUMN lanes.size
    Note over R2: Remove deferred column mappings and unused imports
Loading

Reviews (4): Last reviewed commit: "Drop deprecated lanes.size columns (PP-4..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (c7dbfe9) to head (5b3960c).

Additional details and impacted files
@@                          Coverage Diff                          @@
##           chore/has-root-lanes-scalar-count    #3563      +/-   ##
=====================================================================
- Coverage                              93.52%   93.52%   -0.01%     
=====================================================================
  Files                                    512      512              
  Lines                                  46760    46758       -2     
  Branches                                6379     6379              
=====================================================================
- Hits                                   43731    43729       -2     
  Misses                                  1958     1958              
  Partials                                1071     1071              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathangreen
jonathangreen force-pushed the chore/drop-lane-size-columns branch from 014147b to 20339df Compare July 31, 2026 14:03
@jonathangreen
jonathangreen requested a review from a team July 31, 2026 14:04
@jonathangreen
jonathangreen marked this pull request as ready for review July 31, 2026 14:04
@jonathangreen
jonathangreen force-pushed the chore/drop-lane-size-columns branch from 20339df to f4a7924 Compare August 3, 2026 16:43
@jonathangreen
jonathangreen changed the base branch from main to chore/has-root-lanes-scalar-count August 3, 2026 16:44
@jonathangreen
jonathangreen marked this pull request as draft August 3, 2026 16:44
Release-2 of the online-migration sequence started in the parent PR. Now that a
release marking size / size_by_entrypoint as deferred and moving size's default
into the database (server_default) has shipped, no running release (current or
N-1) reads or writes these columns, so they can be dropped safely.

Removes the columns from the Lane model (and the now-unused deferred / text /
JSON imports) and adds the drop migration. The downgrade recreates the columns
in their post-release-1 state (size NOT NULL with server_default 0,
size_by_entrypoint nullable JSON).

Stacked on the release-1 PR; do not merge until that release has gone out.
@jonathangreen
jonathangreen force-pushed the chore/drop-lane-size-columns branch from f4a7924 to 5b3960c Compare August 3, 2026 17:44
@jonathangreen
jonathangreen requested review from a team and removed request for a team August 3, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DB migration This PR contains a DB migration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant