BACK-627 - Prevent forced allocation refresh from joining an in-flight stale fetch - #925
BACK-627 - Prevent forced allocation refresh from joining an in-flight stale fetch#925jafigueroam94 wants to merge 12 commits into
Conversation
|
@codex review |
![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: 8e58b5635e
ℹ️ 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".
| ...(isCreateMode && assignee.length === 0 && createModeAssignee.length === 0 ? {} : { assignee }), | ||
| labels, | ||
| priority: priority === "" ? undefined : priority, | ||
| project: project === "" ? undefined : project, |
There was a problem hiding this comment.
When a user selects a project in the browser, this value is sent to the real POST/PUT endpoints, but BacklogServer.handleCreateTask does not include payload.project in createTaskFromInput, and handleUpdateTask never copies updates.project into updateInput. Consequently neither creating nor editing a task through the shipped web UI persists the selection; the component tests miss this because they mock apiClient. Add project handling to both server mutation paths.
Useful? React with 👍 / 👎.
| ) | ||
| .option("--priority <priority>", "set task priority (configured priorities)") | ||
| .option("--type <type>", "set task type (configured task types; pass an empty value to clear)") | ||
| .option("--project <project>", "set task project (configured projects; pass an empty value to clear)") |
There was a problem hiding this comment.
In an interactive TTY, backlog task edit TASK-1 --project Web still satisfies !hasEditFieldFlags(options) because that detector was not updated for the new option, so the command opens the full wizard and ignores the requested flag instead of applying it directly. hasCreateFieldFlags has the same omission, causing a title-less create with --project to enter the wizard rather than follow the other explicit-field behavior; add options.project !== undefined to both detectors.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
| const joinedRefreshStartedAt = this.remoteRefRefreshStartedAt; | ||
| await this.remoteRefRefreshPromise; | ||
|
|
||
| if (options?.force !== true || joinedRefreshStartedAt >= requestedAt) return; |
There was a problem hiding this comment.
If a forced allocation request arrives after the stale fetch starts but within the same Date.now() millisecond, joinedRefreshStartedAt equals requestedAt, so this condition treats the older fetch as sufficiently fresh and returns without the follow-up fetch. A push occurring during that fetch can therefore remain invisible and still produce the duplicate ID this change is intended to prevent; track refresh/request ordering with a monotonically incremented generation or promise identity rather than millisecond timestamp comparison.
Useful? React with 👍 / 👎.
| const ProjectBadge: React.FC<ProjectBadgeProps> = ({ project, availableProjects, className = '' }) => { | ||
| const label = project?.trim(); | ||
| if (!label) { |
There was a problem hiding this comment.
When availableProjects is empty but a task still has persisted project frontmatter—for example after removing the projects list from config—this component continues rendering the badge because it only checks the task value. The rest of the new web project controls disappear in that state, leaving visible project UI that cannot be edited or filtered; return null when no projects are configured, as the select and filter already do.
Useful? React with 👍 / 👎.
| cleanup(); | ||
| return; | ||
| } | ||
| filters.project = canonicalProjects; |
There was a problem hiding this comment.
When --project is supplied without an explicit result --type, types has already been calculated at lines 2058-2070 and remains task/document/decision because only modified-file and task-type filters trigger task-only scoping. SearchService applies project filtering only to tasks, so plain and JSON output append every document and decision; the interactive path additionally replaces the filtered task set with allTasks and omits project from the runUnifiedView filter. Parse projects before result-type selection, treat them like --task-type, and pass the project filter into the interactive view.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
| if (baseFilters.project) { | ||
| const projects = Array.isArray(baseFilters.project) ? baseFilters.project : [baseFilters.project]; | ||
| activeFilters.push(`Project: ${projects.join(", ")}`); |
There was a problem hiding this comment.
In a TTY, backlog task list --project Web records this project only in the displayed filter banner, while initialUnifiedFilter at lines 2702-2733 has no project property and the interactive loader deliberately loads the unfiltered corpus. The resulting TUI therefore shows every task under a title claiming Project: Web; include baseFilters.project in the unified filter just as the type filter is included.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
| - `backlog task list --search "desktop app" --labels frontend,bug --limit 20 --plain` | ||
|
|
||
| Avoid broad unfiltered listing when the project may have many tasks. Use `--status`, `--exclude-status`, `--type`, `--assignee`, `--unassigned`, `--parent`, `--priority`, `--labels`, `--search`, or `--limit` where applicable. Repeat `--exclude-status` or pass comma-separated configured statuses to exclude multiple states. Repeat `--type` or pass comma-separated configured task types to include multiple types. | ||
| Avoid broad unfiltered listing when the project may have many tasks. Use `--status`, `--exclude-status`, `--type`, `--project`, `--assignee`, `--unassigned`, `--parent`, `--priority`, `--labels`, `--search`, or `--limit` where applicable. Repeat `--exclude-status` or pass comma-separated configured statuses to exclude multiple states. Repeat `--type` or pass comma-separated configured task types to include multiple types. `--project` filters by configured project in monorepo-style backlogs; it has no effect and is not shown in `--help` when no `projects:` list is configured. |
There was a problem hiding this comment.
This shipped agent instruction says --project has no effect and is absent from help when projects are unconfigured, but normalizeCliProjects exits nonzero and the Commander option remains visible with a no projects configured schema description. Agents following this text can incorrectly treat the flag as a safe no-op; describe the fail-closed error and continued help visibility instead.
AGENTS.md reference: AGENTS.md:L73-L75
Useful? React with 👍 / 👎.
| screen.key(["g", "G"], () => { | ||
| if (modalOpen || filterPopupOpen) return; | ||
| void openFilterPicker("project"); |
There was a problem hiding this comment.
When the task detail pane is focused, lowercase g and uppercase G are already bound at lines 1095-1103 to scroll to the top and bottom. This new screen-level project binding claims the same keys, so key propagation can both scroll and open the picker, or the global handler can make the established detail navigation unavailable whenever projects are configured; choose an unused shortcut or suppress the project handler while the detail pane owns the key.
Useful? React with 👍 / 👎.
8e58b56 to
dbd8fc1
Compare
Adds a validated, single-valued `project` task attribute for monorepo backlogs, following the same pattern as `priority` and `type` but fail-closed: with no `projects:` configured, the field is invalid to set and invisible in every surface (no badge, no filter, no MCP enum). Covers subtasks BACK-637.1 through BACK-637.4: - Core: Task.project, frontmatter serialization, Core.normalizeProject, config `projects:` list (src/utils/project-config.ts) - CLI: --project on task create/edit, config get/list, --help, task wizard, shell completions, plain/JSON output - MCP: project on task_create/task_edit with schema field omitted when unconfigured - Filtering: --project on task list/search (OR semantics, matching --type), Core/ContentStore/FileSystem/SearchService/task-search all wired consistently, MCP task_list/task_search, GET /api/search TUI (BACK-637.5) and Web UI (BACK-637.6) are tracked as follow-up subtasks under the BACK-637 parent.
… views Adds the project attribute to the terminal UI: badges on board cards and list rows, a Project: line in task detail view, a filter-header control mirroring --type's multi-value OR semantics through the shared unified-view filter state, a G keyboard shortcut, and a task-composer field. Every project UI element is conditionally hidden when no projects are configured, matching the fail-closed design from the core/CLI/MCP work. The task composer renders project as its own full-width row below type/priority rather than recalculating the existing 3-column compact/expanded layout math, keeping the change isolated to a single extra row in detailsHeight when configured.
Adds the project attribute to the web UI, scoped to where the type attribute already lives (Board/BoardPage/TaskCard/TaskDetailsModal), after confirming via source that TaskList.tsx and DraftsList.tsx have no type support to mirror. New ProjectBadge.tsx component; availableProjects threaded from App.tsx through BoardPage's URL-param-driven filter down to TaskCard's badge; TaskDetailsModal gained a project select using priority's simple immediate-save pattern. All project UI is absent when config.projects is empty.
The compact task-list JSON envelope test asserted an exact object shape via toEqual, which broke once BACK-637.2 added project: null to TaskSummaryJson. Add the field to the expected fixture; document/ decision search results are unaffected since project is a task-only field.
… to end - search/task list: --project now scopes result types like --task-type, is forwarded into the interactive view's filter and filter description, and seeds the TUI's tasksLoader query (previously loaded unfiltered) - hasCreateFieldFlags/hasEditFieldFlags now recognize --project, so it no longer forces the interactive wizard open on a TTY - "No projects are configured" now interpolates the actual resolved config path via a shared noProjectsConfiguredMessage() helper instead of a hardcoded backlog/config.yml, at all 3 call sites - Web UI can now clear an existing project on edit (project: null flows through TaskDetailsModal -> api.ts -> server handlers -> Core, mirroring the existing dueDate/milestone clear pattern) - server/index.ts handleCreateTask/handleUpdateTask now forward payload project into Core, closing the gap where the web UI's project field was a no-op end to end (new server-task-project-endpoint.test.ts covers this, verified to fail without the fix) - Project badges (web ProjectBadge, TUI formatProjectBadge and its board list/detail-pane/popup callers) now stay hidden when no projects are configured, even if a task still carries stale project frontmatter - Moved the project filter picker off g/G in both board and task-list TUI views (now v/V) to stop colliding with the pre-existing detail-pane scroll-to-top/bottom shortcuts; updated the help overlay to match - Corrected the shipped task-creation.md guidance, which claimed --project is hidden from --help and a no-op when unconfigured (it's always shown and fails closed with a nonzero exit)
112bded to
74b1d58
Compare
…t stale fetch A forced allocation refresh that joined an already-in-flight non-forced fetch returned without observing a push that landed during that fetch's remaining duration, risking a duplicate task ID. The force path now tracks when the joined refresh started and issues one more fetch if it started before the force request.
…tion counter Date.now()-based comparison could misjudge freshness when a forced request and the in-flight fetch's start both land in the same millisecond, treating a stale fetch as sufficiently fresh. Replace remoteRefRefreshStartedAt/requestedAt timestamps with a monotonically incremented remoteRefRefreshGeneration counter for the loop's join-vs-refetch decision. disposeContentStore no longer resets the counter (resetting a monotonic counter would make a caller holding a stale high generation loop through extra real fetches before exiting).
32fbbbd to
220bc80
Compare
2 participants
Summary
Follow-up from the PR #899 (BACK-624) verification round. The task-ID allocation path forces a remote-ref refresh past the normal 60s lease, but a forced refresh arriving while a non-forced fetch is already in flight simply joined that fetch and returned — a push landing during the remaining fetch duration was invisible to the allocation, allowing a duplicate numeric ID (window typically under 2s, capped at 10s).
Core.refreshRemoteRefsForTaskRead(src/core/backlog.ts) now tracks when the in-flight refresh it joins actually started (remoteRefRefreshStartedAt).Closes BACK-627.
Test plan
src/test/core-task-corpus-regressions.test.tsreproduces the push-during-in-flight-fetch race with a gatedgit.fetchmock (real fetch data capture happens before the push, resolution withheld until released) driving a concurrent forcedgenerateNextId()callTASK-2) and passes post-fix (allocatesTASK-3via exactly 2 fetches)bunx tsc --noEmitcleanbun run check .clean (391 files)bun run test— 2395 pass / 6 pre-existing skips / 0 fail across 250 files