Skip to content

fix(models): make session-index rebuild thread ownership explicit (#3894) - #4993

Closed
rodboev wants to merge 1 commit into
nesquena:masterfrom
rodboev:pr/3894-index-rebuild-thread-handoff
Closed

rodboev wants to merge 1 commit into
nesquena:masterfrom
rodboev:pr/3894-index-rebuild-thread-handoff

Conversation

@rodboev

@rodboev rodboev commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • #3894 is a bookkeeping hardening follow-up, not a user-visible bug report. The safest change is the one the issue already points at: make cleanup depend on explicit ownership, not just a matching target tuple.
  • api/models.py already records both the current owner thread and the current target. The missing guard is simply that the worker finishing never checks whether it is still the registered owner before clearing those globals.
  • This PR keeps the rebuild model intact. It only makes the owner handoff explicit and locks the latent same-target handoff window with a deterministic regression test.

What Changed

  • api/models.py: captures the current worker thread in _rebuild_session_index_background(...) and clears the rebuild globals only when the finishing worker is still the registered owner for the matching target tuple.
  • tests/test_session_index.py: adds a deterministic same-target handoff regression that proves an older worker no longer clears a newer owner's globals.

Why It Matters

This closes a subtle ownership window in the background rebuild bookkeeping without changing scheduling or user-visible behavior. The code becomes explicit about who is allowed to clear the shared globals, which makes future thread-hand-off work safer.

Verification

pytest tests/test_session_index.py -v --timeout=60
pytest tests/test_issue2863_session_index_prime.py -v --timeout=60

Full-suite CI context, not a required local check unless requested: pytest tests/ -v --timeout=60.

Upstream

Closes #3894.

Follow-up to #3884, which introduced the target-pinned rebuild ownership this PR hardens.

Model Used

GPT 5.5 via Codex CLI

@greptile-apps

greptile-apps Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens the background session-index rebuild bookkeeping in api/models.py by making thread ownership explicit before clearing shared globals. Previously, the finally block cleared _SESSION_INDEX_REBUILD_THREAD and _SESSION_INDEX_REBUILD_THREAD_TARGET whenever the target tuple matched, which left a window where an older finishing worker could silently clobber ownership state already handed off to a newer worker for the same target.

  • api/models.py: captures threading.current_thread() at function entry and gates the finally-block cleanup on _SESSION_INDEX_REBUILD_THREAD is current_thread (identity) in addition to the existing target-tuple check.
  • tests/test_session_index.py: adds a deterministic regression test that simulates an in-flight ownership handoff (same target, new thread object) and asserts the older finishing worker no longer clears the new owner's globals.

Confidence Score: 5/5

Safe to merge — the change is a minimal two-line guard on an existing cleanup path with no effect on scheduling or user-visible behavior.

The fix is narrow and self-contained: one threading.current_thread() capture and one added identity predicate in the finally block. The accompanying regression test is deterministic and correctly exercises the same-target handoff scenario. No scheduling logic, no public API surface, and no data paths are altered.

No files require special attention.

Important Files Changed

Filename Overview
api/models.py Adds a single-line current_thread capture before the try-block and adds an identity guard (is current_thread) to the finally-block cleanup — minimal, correct hardening of the same-target handoff window.
tests/test_session_index.py Adds a deterministic regression test that patches threading.current_thread and simulates a same-target ownership handoff during _write_session_index, then asserts the older worker does not clear the new owner's globals.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Scheduler as _start_session_index_rebuild_thread
    participant Old as OldWorker (Thread A)
    participant New as NewWorker (Thread B)
    participant Globals as Shared Globals

    Scheduler->>Globals: "SET thread=A, target=(dir, idx)"
    Scheduler->>Old: start()
    Old->>Old: "current_thread = A"
    Old->>Old: acquire lock, target matches, release lock
    Old->>Old: _write_session_index() [slow I/O]

    Note over Scheduler,Globals: Second rebuild triggered (same target)
    Scheduler->>Globals: "SET thread=B, target=(dir, idx)"
    Scheduler->>New: start()

    Old->>Old: finally: acquire lock
    Old->>Old: _SESSION_INDEX_REBUILD_THREAD is A? NO (it is B)
    Old->>Old: skip clear, release lock

    Note over Globals: B still registered as owner

    New->>New: "current_thread = B"
    New->>New: _write_session_index()
    New->>New: finally: _SESSION_INDEX_REBUILD_THREAD is B? YES
    New->>Globals: "SET thread=None, target=None"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Scheduler as _start_session_index_rebuild_thread
    participant Old as OldWorker (Thread A)
    participant New as NewWorker (Thread B)
    participant Globals as Shared Globals

    Scheduler->>Globals: "SET thread=A, target=(dir, idx)"
    Scheduler->>Old: start()
    Old->>Old: "current_thread = A"
    Old->>Old: acquire lock, target matches, release lock
    Old->>Old: _write_session_index() [slow I/O]

    Note over Scheduler,Globals: Second rebuild triggered (same target)
    Scheduler->>Globals: "SET thread=B, target=(dir, idx)"
    Scheduler->>New: start()

    Old->>Old: finally: acquire lock
    Old->>Old: _SESSION_INDEX_REBUILD_THREAD is A? NO (it is B)
    Old->>Old: skip clear, release lock

    Note over Globals: B still registered as owner

    New->>New: "current_thread = B"
    New->>New: _write_session_index()
    New->>New: finally: _SESSION_INDEX_REBUILD_THREAD is B? YES
    New->>Globals: "SET thread=None, target=None"
Loading

Reviews (1): Last reviewed commit: "fix(models): make session-index rebuild ..." | Re-trigger Greptile

@nesquena-hermes nesquena-hermes added the size:S Small PR (≤2 files, ≤30 LOC) label Jun 26, 2026
nesquena-hermes added a commit that referenced this pull request Jun 26, 2026
…ip (#4993, #3894)

Release YJ (v0.51.680): explicit session-index rebuild thread ownership (#4993, #3894)
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in v0.51.680 (Release YJ, just deployed) — thanks @rodboev! Addresses #3894. The session-index rebuild finally-block now requires the finishing worker to still be the registered owner thread before clearing the bookkeeping globals, so a late older worker can't clobber a newer rebuild's state. Gate: Codex SAFE (under-lock, no TOCTOU, no stuck-owner leak), full suite 10693. Verified on prod.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in v0.51.680 (explicit session-index rebuild thread ownership) — the fix is live on master and deployed to prod. Thanks @rodboev for the contribution. Closing as shipped (the release landed via a stage/release branch so this PR object didn't auto-close). Please reopen if you still see the issue after upgrading + a hard refresh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S Small PR (≤2 files, ≤30 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden index-rebuild thread ownership handoff (low-severity follow-up to #3884)

2 participants